Implement sending of allocated validator payments #11197

m-chrzan · 2024-08-23T15:06:53Z

Description

Implements sendValidatorPayment(address validator), which sends the cUSD validator payment allocated via allocateValidatorRewards to the validator, group, and delegation beneficiary.

TODO:

Add ERC-20 checks
Double check if any other checks present in distributeEpochPaymentsFromSigner need to be carried over
- They're covered by computeEpochReward ✅
Emit an event
NatSpecs

Questions:

Seems like when we added payment delegation, we didn't add the beneficiary payment to the event emitted (presumably for backwards compatibility reasons?). Do we want to add it now (or emit an additional event with delegation payment information)?
- As discussed with @mcortesi in comments below, we'll add delegation info.

Follow-ups:

Call this in functions that would modify the amount of payment distributed (https://github.com/celo-org/celo-blockchain-planning/issues/552)

Other changes

Changes to various mock contracts. In particular:

The old 0.5 MockValidators.sol should probably be unified with ValidatorsMock08.sol once Move Validators.sol to 0.8 #11192 is merged. The newly added IMockValidators.sol (necessary to use an 0.5 contract in an 0.8 test) can be removed at that point.
EpochManager_WithMocks.sol extends the base EpochManager contract, and is used as the main contract in EpochManager tests. It behaves like the original contract, but adds a function to manually set payment allocations for easier testing.

Tested

Forge unit tests.

Related issues

Fixes https://github.com/celo-org/celo-blockchain-planning/issues/555

Documentation

Should add docs for validators on how to call this function to claim their payments.

pahor167 · 2024-08-28T13:27:45Z

packages/protocol/contracts-0.8/common/EpochManager.sol

+   */
+  function sendValidatorPayment(address validator) external {
+    IAccounts accounts = IAccounts(getAccounts());
+    address signer = accounts.getValidatorSigner(validator);


I believe we didn't want to use signers @mcortesi @martinvol

I was basing this on how the current code works:

the elected array in EpochManager gets set by calling Election.electNValidatorSigners

That functions gets validator addresses via Validators.getTopGroupValidators

Which grabs the validator accounts and translates them to validator signer addresses, returning those

So in the current state, the correct way to index into elected is via signer addresses.

If we want to change this logic, we should thoroughly double check everything is returning/using the correct type of validator address. As mentioned on Slack, a similar issue exists with the current use of Validators.computeEpochReward, which expects a validator account, but gets a signer address (generated by the election). I would suggest this be done in a follow-up issue/PR that covers all instances of this.

In general I agree, we can probably just use account addresses everywhere. If I understand correctly, the only reason we used signer addresses in some places was because that's what celo-blockchain cared about in the PoS logic.

yes, we don't want signeers. EpochManager will have a list of elected account (not signers) and we should change elections to not give signers, i believe we even have a draft methos doing that...

the client didn't care about account, only about signers and so it sent signers everywhere, but that's unnecessary complexity now

Created an issue to track progress on this

@mcortesi, sounds great, that's how I understood things.

As mentioned, I would merge this PR as is, so it's consistent with the current state of the logic, and remove signers in the PR that will fix @soloseng's issue.

PR making the changes.

packages/protocol/contracts-0.8/common/EpochManager.sol

mcortesi · 2024-08-28T20:09:15Z

packages/protocol/contracts-0.8/common/EpochManager.sol

+
+    IStableToken stableToken = IStableToken(getStableToken());
+
+    require(stableToken.transfer(validator, validatorPayment), "mint failed to validator");


not mint but transfer. Can transfer fail?

is validatorPayment always > 0?

Can transfer fail?

If the contract doesn't have enough balance for example. Ideally that should never happen though, if our allocation and minting logic is correct. Auditors in the past have asked to always check return values of e.g. ERC-20 functions that return a success bool, as good practice.

is validatorPayment always > 0?

I guess it could be 0, if delegation and/or commission take up the whole payment. I can add an if(non-zero) around this as well. That said, transfers of 0 do not fail, as evidenced by the test_doesNothingIfNotAllocated test not reverting.

mcortesi · 2024-08-28T20:11:16Z

packages/protocol/contracts-0.8/common/EpochManager.sol

+      require(stableToken.transfer(beneficiary, delegatedPayment), "mint failed to delegatee");
+    }
+
+    emit ValidatorEpochPaymentDistributed(validator, validatorPayment, group, groupPayment);


this looks broken, even in Validators.sol. We record validator and group payments but not delegatee. Since this is a new event on a new contract, we should correct that

mcortesi · 2024-08-28T20:13:04Z

packages/protocol/contracts-0.8/common/EpochManager.sol

+
+    require(stableToken.transfer(validator, validatorPayment), "mint failed to validator");
+    if (groupPayment > 0) {
+      require(stableToken.transfer(group, groupPayment), "mint failed to validator group");


not mint but transfer

mcortesi · 2024-08-28T20:13:10Z

packages/protocol/contracts-0.8/common/EpochManager.sol

+      require(stableToken.transfer(group, groupPayment), "mint failed to validator group");
+    }
+    if (delegatedPayment > 0) {
+      require(stableToken.transfer(beneficiary, delegatedPayment), "mint failed to delegatee");


not mint but transfer

soloseng

🚀

pahor167 · 2024-09-16T18:48:14Z

packages/protocol/contracts-0.8/common/EpochManager.sol

+    FixidityLib.Fraction memory remainingPayment = FixidityLib.newFixed(
+      totalPayment.fromFixed() - groupPayment
+    );
+    (address beneficiary, uint256 fraction) = getAccounts().getPaymentDelegation(validator);


Suggested change

(address beneficiary, uint256 fraction) = getAccounts().getPaymentDelegation(validator);

(address beneficiary, uint256 delegationFraction) = getAccounts().getPaymentDelegation(validator);

So it is more clear what does it stand for ?

* first compiling draft no working test. Co-authored-by: Martín Volpe <[email protected]> * Compiler fixes + finishNextEpochProcess * allocateValidatorsRewards compilable * First few tests * test fix * uisng registry instead * updated registry ID var names * typo * ++ mock contracts in 0.8 * ++ passing test using mock * Removal of using precompiles * ScoreManager refactor * Decouple epoch manager initializer from registry * isReadyToStartEpoch fix * startNextEpochProcess fixes * rename score manager to score reader * Rename transfer to release on CeloDistribution schedule * Celo distribution schedule renamed to CeloUnreleasedTreasure * conditions to getFirstBlockAtEpoch and getLastBlockAtEpoch * ScoreManager update * getFirstBlockOfEpoch in EpochManager initializer * extra checks * introduce new getValidatorsGroup * systemAlreadyInitialized update * Removal of IEpochManagerInterface * Made CI run on feature branch * merge fix * Added target for the CI * Fixed quote celo-monorepo.yml * Allow Validator registration in L2 without BLS key (#11181) * Allow validator registration in L2 without BLS key * Reallow ECDSA key change in L2 --------- Co-authored-by: Martín Volpe <[email protected]> * Fix CI * Fixed interfaces and imports * Build fix * lint * prettify * prettify 2 * Attempt to fix truffle build and migrations l2 epoch (#11190) * Added Blocking to LockedGold, Election (#11186) * add EpochManager to abis * force deploy abis * build fix * Move Validators.sol to 0.8 (#11192) * WIP * Validator test WIP, forge doesn't compile yet * buildable * most of the foundry tests working * Validator related tests fixed * truffle build working * cache bump * lint + prettify+ migrations * Enable optimization for Solidity 0.8 * prettify * Ci bump * CI bump 2 * Validators to 0.8 config * CI bump * foundry fix * Truffle migrations are partly fixed * Added import for ValidatorsMock08 in EpochManager.t.sol, I think it was removed by mistake * Added yarn.lock * Changes to mock with full implementation * Attempt to fix linking libraries not working with deployCodeTo foundry-rs/foundry#4049 * truffle migrations fixed * CI bump * lint * forge test fixes * artifacts test fix * lint * update of foundry version * add ProxyFactory import to tests * library linking fix * Foundry migrations fix * migration tests fix * CI bump * Little cleanup + retrigger CI * forgot to commit Validators.sol * Fixed the ABI encoded * lint * Fix contract versions * add Adapter to ignored contracts * revert of ReentrancyGuard change * lint fix * remove adapters from check * storage layout fix --------- Co-authored-by: pahor167 <[email protected]> * force release of canary abi * force new canary version * npm tag revert * Soloseng/update-l2-getepochnumber-logic (#11195) * disable getEpochSize on L2 * update registry * update relevant interfaces * update contracts with L2 `getEpochNumber()` logic * update tests with L2 `getEpochNumber()` logic * ++ TODO * moved constants to constants file * updated allocated supply function to handle L1 & L2 cases * made `epochManager.currenEpochNumber()` private, to avoid returning 0 when not initialized. * PR feedback * Passing validators test using mockEpochManager for L2 tests * clean up * fixed other failing tests * using mockEpochManager instead of interface. Fixed failing tests. * happy linter * revert npm tag ∆ * ++ TODO and comment * add score manager to abis * Split initEpochManager Function (#11199) * ++ contract function * ++ comment * startNextEpochProcess unit & integration test (#11191) * unit test with mocks * ++ integration tests * clean up * -- logging * removed duplicate interface * using `MockCeloToken` to get test to pass. Fails when it hits a precompile in `EpochRewards.sol` * removed endEpochTimestamp * moved IEpochManager to 0.5 folder * added L2 conditions for EpochRewards functions using precompiles Still missing tests * renamed EpochManagerInitializer due to name conflict * ++ more unit test * setup anvil migration fix name conflict * compiles * ++ require fund in unreleased treasury * Updated regex * ++ registry 0.8 for testing only * clean up * ++ unit test * initial integration test using L1 devchain * ++ comment * -- forge based integration test * ++ to const * happy linter * update contract name * ++ PR feedback * ++ checks * updated carbon address * proxy stableToken mint call via Validators contract * -- duplicate imports * removed registry08. replaced with vm call * PR feedback * -- coment * passing unit tests * clean up * ++ mintStable test * -- TODO; compiles test when filtering * PR feedback * updated migration script to add more validators * passing integration test * removed test for zero amount * yarn build fix * clean up comments && TODO * revert change as out of scope * E2E EpochManager test + Epoch truffle migrations & Anvil L2 migration build fix (#11198) * Soloseng/dynamically-fetch-epochmanagerenabler-address (#11207) * dynamically fetch epochManagerEnabler && carbonOffsettingPartner ++ to registry * PR feedback * removed onlyL1 modifier in setter functions * updated unit test to reflect changes * fixing tests * fix test * fixed migration data * fixed migration script error * removed unused modifier * removed duplicate or unused code * Implement sending of allocated validator payments (#11197) * EpochManager fixes (#11208) * truffle build fix * build fix * PR comments * prettify * rename of registerValidator overload * bug fix * extensing epochManager e2e test * yarn lint * Reset pending payment to 0 after sending (#11209) Reset pending payment to 0 when sending * Deleted duplicated import * Make captureEpochAndValidators work in constant time (#11210) * Use more general interface for token transfer (#11216) * Epoch manager enabler tests (#11213) * ++ basic test * -- celoToken balance check * cleanup comments * use celoToken instead of native token for `initializeSystem` balance check * ++ more test * removed additional epochs * Make sendValidatorPayment nonReentrant (#11217) * Martinvol/return account instead of signer (#11215) * EpochManager e2e tests add/remove validators between ecpochs (#11214) * Send validator payment on actions that would modify reward distribution (#11211) * Rename `CeloUnreleasedTreasure` to `CeloUnreleasedTreasury` (#11220) * updated `CeloUnrealeasedTreasure` to `CeloUnreleasedTreasury` * update `CeloUnreleasedTreasury` initial balance to use defined constant * renamed file * prevent CELO transfers to `CeloUnreleasedTreasury` * ++ comment * unused import * using internal accounting for released amount * updated integration test * PR feedback * PR feedback * reverted due to gas consideration * more revert --------- Co-authored-by: Martín Volpe <[email protected]> Co-authored-by: pahor167 <[email protected]> Co-authored-by: Martín Volpe <[email protected]> Co-authored-by: Martín Volpe <[email protected]> Co-authored-by: Martin <[email protected]> Co-authored-by: pahor167 <[email protected]>

m-chrzan added 14 commits August 23, 2024 14:44

Set up mocking for payment sending

78cc1a6

Set up basic test for sending payment

cb6eded

Send commission to group

0813287

Send payment to beneficiary

a853665

Remove unnecessary test case

ee1eb46

Test sending to both group and beneficiary

ff0c20b

Test when not allocated

4d73b88

Cleanup test

54a1037

Merge branch 'feat/l2-epoch-system' into m-chrzan/send-validator-payment

80873ba

Check ERC-20 transfer return values

195e718

Emit an event on payment sent

e1beb8f

Add natspecs

771bdfb

Merge branch 'feat/l2-epoch-system' into m-chrzan/send-validator-payment

07839eb

Get pending payment based on signer

8889bf3

m-chrzan marked this pull request as ready for review August 26, 2024 20:47

m-chrzan requested a review from a team as a code owner August 26, 2024 20:47

Use relative import

78c5303

m-chrzan changed the title ~~[WIP] Implement sending of allocated validator payments~~ Implement sending of allocated validator payments Aug 26, 2024

Move v8 interface to v8 directory

e0493cf

pahor167 reviewed Aug 28, 2024

View reviewed changes

mcortesi reviewed Aug 28, 2024

View reviewed changes

m-chrzan added 4 commits August 30, 2024 13:17

Merge branch 'feat/l2-epoch-system' into m-chrzan/send-validator-payment

1cb64bf

Correct mint to transfer

7da47c6

Add delegation info to event

624e348

Don't transfer to validator if payment is 0

2a29cd1

soloseng approved these changes Sep 13, 2024

View reviewed changes

pahor167 reviewed Sep 16, 2024

View reviewed changes

m-chrzan and others added 3 commits September 17, 2024 16:31

Merge branch 'feat/l2-epoch-system' into m-chrzan/send-validator-payment

3df09eb

Rename variable

1a50a3f

Merge branch 'feat/l2-epoch-system' into m-chrzan/send-validator-payment

b47fd27

martinvol and others added 2 commits September 17, 2024 18:50

Trigger CI

34d8949

Merge branch 'feat/l2-epoch-system' into m-chrzan/send-validator-payment

85f9957

martinvol merged commit 59ea380 into feat/l2-epoch-system Sep 18, 2024
21 checks passed

martinvol deleted the m-chrzan/send-validator-payment branch September 18, 2024 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement sending of allocated validator payments #11197

Implement sending of allocated validator payments #11197

m-chrzan commented Aug 23, 2024 •

edited by martinvol

Loading

pahor167 Aug 28, 2024

m-chrzan Aug 28, 2024

mcortesi Aug 28, 2024

soloseng Aug 29, 2024

m-chrzan Aug 30, 2024

soloseng Sep 13, 2024

mcortesi Aug 28, 2024

mcortesi Aug 28, 2024

m-chrzan Aug 30, 2024 •

edited

Loading

mcortesi Aug 28, 2024

mcortesi Aug 28, 2024

mcortesi Aug 28, 2024

soloseng left a comment

pahor167 Sep 16, 2024


		IStableToken stableToken = IStableToken(getStableToken());

		require(stableToken.transfer(validator, validatorPayment), "mint failed to validator");

	(address beneficiary, uint256 fraction) = getAccounts().getPaymentDelegation(validator);
	(address beneficiary, uint256 delegationFraction) = getAccounts().getPaymentDelegation(validator);

Implement sending of allocated validator payments #11197

Implement sending of allocated validator payments #11197

Conversation

m-chrzan commented Aug 23, 2024 • edited by martinvol Loading

Description

Other changes

Tested

Related issues

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-chrzan Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soloseng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-chrzan commented Aug 23, 2024 •

edited by martinvol

Loading

m-chrzan Aug 30, 2024 •

edited

Loading